Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Line2D.DistanceTo(Point2D p) #238

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

f-frhs
Copy link
Contributor

@f-frhs f-frhs commented Dec 25, 2023

I implemented Line2D.DistanceTo(Point2D p) which calculates the distance from this line to the given point p.
This method should always return the value of 0 or positive.

It will be nice if we can discuss whether the testcases is sufficient.

new object[] { "+1,-1", "+1,+1", "3,0", 2.0 },
};

[TestCaseSource(nameof(DistanceTestCases))]
Copy link
Member

@jkalias jkalias Dec 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the previous structure with TestCase was much better. My suggestion with the swapped scenarios was meant to be handled in the TestCase definition, not as a separate test function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment.
Could you tell me why using TestCode is better than using TestCodeSource for this test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the whole new object[] a bit overkill, that's it. I guess it's more a matter of personal taste.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment.

it's more a matter of personal taste.

Could you show the reason?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for what exactly? I prefer compile-time checks and try to avoid object as much as possible

Copy link
Contributor Author

@f-frhs f-frhs Jan 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for telling the reasons.

Done in the commit cd80cf1.
I changed the test-code as you required. I'm not sure that the revised test-code helps the reader understand at a glance that parameters are kept intact except its order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. This is not what I meant, we can augment the existing tests with a reversed line

var line = new Line2D(Point2D.Parse(p1s), Point2D.Parse(p2s));
// existing test

var reversedLine = new Line2D(Point2D.Parse(p2s), Point2D.Parse(p1s));
// distance test with reversed line

This way you keep the TestCase cases at a minimum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for showing the example closely.

It is not recommended to test 2 things in a single unit-test.
Or do you want to say that these 2 tests are equivalent because of the symmetry?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not recommended to test 2 things in a single unit-test.

Fully agree

Or do you want to say that these 2 tests are equivalent because of the symmetry?

Basically yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants